Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(gatsby-plugin-sharp): Base64 features #24999

Merged

Conversation

polarathene
Copy link
Contributor

@polarathene polarathene commented Jun 15, 2020

Description

Documents some base64 features that Gatsby has supported for a while now. These weren't easily discoverable or clear how to use them.

I did some minor refactoring related to these features. The relevant individual commits provide details in their commit messages.

Documentation

The graphql parameters toFormatBase64 and base64Width are now documented in the gatsby-plugin-sharp README. The global override equivalent options are mentioned for each, but not documented elsewhere.

@gatsbyjs/learning is there any additional documentation of these that would be appreciated? A code snippet or PR to a project that demonstrates the minor features?

Improvements

Some code refactoring, fixes and tests added.

Also background option is now passed to base64(), fixes #25640

@polarathene polarathene requested review from a team as code owners June 15, 2020 04:52
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 15, 2020
@wardpeet wardpeet self-assigned this Jun 16, 2020
@wardpeet wardpeet added status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 16, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! One small comment

packages/gatsby-plugin-sharp/README.md Outdated Show resolved Hide resolved
@wardpeet wardpeet changed the title Docs(gatsby-plugin-sharp): Base64 features docs(gatsby-plugin-sharp): Base64 features Jul 3, 2020
wardpeet
wardpeet previously approved these changes Jul 3, 2020
@wardpeet wardpeet added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: needs core review Currently awaiting review from Core team member labels Jul 3, 2020
@LekoArts
Copy link
Contributor

LekoArts commented Jul 3, 2020

Seems like tests are failing cc @wardpeet @polarathene

@wardpeet wardpeet added status: awaiting author response Additional information has been requested from the author and removed bot: merge on green Gatsbot will merge these PRs automatically when all tests passes labels Jul 3, 2020
@polarathene

This comment has been minimized.

@polarathene

This comment has been minimized.

polarathene and others added 6 commits July 11, 2020 23:34
This feature was added when an internal fixed value of `20` was in use. It was configurable via graphql query parameters as well as overriding the default value in the plugin config, but this was not obvious as no default was set.

As the default is `20`, it has been relocated to the plugin defaults, and the redundant method to get the `base64Width` value has been removed(this was implemented before the plugin defaults and options were refactored out to a separate file).
Using a default of `false` is a bit misleading as the expected values are `jpg`,`png` and `webp`, not `true`. The empty string is a falsey value, serving the same purpose. This matches the defaults for similar properties in general args.

Additionally updates the docs to show the actual default values and missing options for plugin config options, as these lacked public documentation.
Documents these two parameters for graphql (and their global default override option equivalents via plugin config).
`args` is a parameter, it should not have it's props modified, referencing the `options` object instead where `toFormat` may be modified.

Rename the `forceBase64Format` variable to `changedBase64Format`. Should be less likely to imply a boolean, and isn't necessarily the same value as `forceBase64Format` stored on the `pluginOptions`.
@polarathene polarathene force-pushed the docs/gatsby-sharp-base64-features branch from 79ea95a to 364bbe5 Compare July 11, 2020 11:36
@polarathene

This comment has been minimized.

@polarathene polarathene removed the status: awaiting author response Additional information has been requested from the author label Jul 11, 2020
`healOptions()` needed an update to pull in the `base64Width` default as fallback.

base64 tests also needed modifications to avoid caching from calling `base64()` directly instead of via `fixed()` and `fluid()` methods which would invalidate cache and set `width` prop to `base64Width`.
@polarathene
Copy link
Contributor Author

All tests should be passing, "Cloud Tests" seems to be hanging for some reason.

Important for transparent images like PNG when converted to base64 image format like JPG where transparency is not supported.

Test is fairly basic relying on snapshot but should be sufficient to indicate if something goes wrong. Could possibly benefit with tests on fluid/fixed where the actual bug was from them not passing the option down.
@polarathene
Copy link
Contributor Author

Added a fix for background option not being passed from fluid/fixed methods to the base64 method.

Included a basic test , although like the base64 tests I've added I'm not sure if they're sufficient, should I also be testing these via fluid/fixed tests instead of directly on base64? The bug for background was simply that the option wasn't passed on to the method, is it worth adding tests for that?

I'm not sure if I can mock the base64 method called in that test file for a single test that ensures supported options are passed through?

@polarathene
Copy link
Contributor Author

@wardpeet Could you review/merge please?

@polarathene
Copy link
Contributor Author

@wardpeet @pvdz hello, friendly reminder request to take the next steps in resolving this PR, thank you.

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added one question, looks good besides that. Sorry to keep you waiting

@polarathene
Copy link
Contributor Author

Added one question, looks good besides that.

@wardpeet where is that question?

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome! Great job getting this done!

@wardpeet wardpeet merged commit 3ad780e into gatsbyjs:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gatsby Image & Sharp background blur-up
4 participants